Skip to content

Deprecation on implicit bool conversions to true for values other than 1 #8565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from

Conversation

iquito
Copy link

@iquito iquito commented May 16, 2022

This is a preliminary implementation of an upcoming RFC about Stricter implicit boolean coercions (see the previous discussion on https://externals.io/message/117608).

This is my first attempt at a PHP contribution, which is why I tried to mimick what the implicit float-to-int deprecation implementation did (#6661). I added some additional tests and adapted the existing tests where deprecation notices appeared.

EDIT: Only two tests are still failing in the AppVeyor build (to do with large ints appearing as -1 - see a comment further below for details). If anything else needs to be changed or can be improved I welcome any feedback. The RFC can be found on https://wiki.php.net/rfc/stricter_implicit_boolean_coercions

@cmb69 cmb69 added the RFC label May 16, 2022
@@ -836,6 +836,19 @@ ZEND_API void ZEND_COLD zend_incompatible_string_to_long_error(const zend_string
zend_error(E_DEPRECATED, "Implicit conversion from float-string \"%s\" to int loses precision", ZSTR_VAL(s));
}

ZEND_API void ZEND_COLD zend_incompatible_double_to_bool_error(double d)
{
zend_error_unchecked(E_DEPRECATED, "Implicit conversion from float %.*H to true, only 0 or 1 are allowed", -1, d);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry if I missed something but why not "0.0 or 1.0"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be an option too (and that was the initial message). I changed it to say 0 or 1 for both float and int to steer a developer towards thinking about the value and not the type, as explicitely using 0.0 or 1.0 as a boolean argument seems like dodgy advice.

@iquito
Copy link
Author

iquito commented May 16, 2022

I fixed the failing iterator tests so they are not affected by this deprecation, and I created the RFC which contains more details: https://wiki.php.net/rfc/stricter_implicit_boolean_coercions

@iquito
Copy link
Author

iquito commented May 16, 2022

A deprecation notice appeared on Travis in run-tests.php, where "failed" or "success" were given to a bool parameter (which should have been string parameter instead), as if trying to highlight coercion problems ;-) I changed it to a string parameter. The mail_qa_team function now posts the status with "success" and "failed" (instead of always having status 1) as it probably should, although that change could be done separately from this RFC of course.

@Girgias
Copy link
Member

Girgias commented May 17, 2022

I'm expecting OpCache to need to have changes made as it now needs to bail out off certain optimizations.

@iquito
Copy link
Author

iquito commented May 17, 2022

I started to look at Opcache and if I need to change anything there, but nothing stuck out so far (but remember, I am super new to the php-src codebase). Are there tests for Opcache/JIT for something like this, or would it make sense to add more tests specifically for something like this to opcache/jit?

I managed to fix all other existing tests with the exception of the AppVeyor build: there are just two failures left, one failure is in scalar_basic.phpt where PHP_INT_MAX shows up as -1 in the deprecation notice not satisfying the pattern %d, and in scalar_return_basic_64bit.phpt where 9223372036854775807 shows up as -1 instead of 9223372036854775807. On the other testing platforms this works as expected (and also locally), and me not knowing the reason for this is probably just showing my lacking C skills, but reporting the correct integer that lead to the notice (and not -1) would be helpful. Any hints on how to best resolve this would be appreciated.

@Girgias
Copy link
Member

Girgias commented May 17, 2022

Compile with CFLAGS="-DPROFITABILITY_CHECKS=0" and the run: make test TEST_PHP_ARGS="-q -j22 -d opcache.jit=function -d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.file_update_protection=0 -d opcache.jit_buffer_size=1M" TESTS="Zend ext/opcache" this should test the JIT and likely find OpCache issues

@iquito
Copy link
Author

iquito commented May 17, 2022

Thanks! I did both and all the tests passed.

@Girgias
Copy link
Member

Girgias commented Jun 20, 2022

RFC has been declined

@Girgias Girgias closed this Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants